Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] Move handling of form elements back to the Form #17981

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 28, 2020

Overview

Minor code cleanup - move generation of an array required to add the checkboxes for 'Move related contributions' etc for the form back to the form

Before

Screen Shot 2020-07-28 at 2 36 11 PM

After

No change - ie
Screen Shot 2020-07-28 at 2 36 11 PM

Technical Details

This toxic function getRowsElementsAndInfo does things for the form layer but it's data also heavily used
in the BAO dedupeProcess. These are the only 2 places that call it

Ideally we want the things that are only being done to support (one specific) form
to sit on that form. This does that just for one of the 2 arrays it creates of form elements.

The rel_table_elements array is just an array that looks like

[
  ['checkbox',  'move_rel_contributions'],
  ['checkbox, 'move_rel_activities'],
]

The rel_tables array is an array of metadata keyed by eg. move_rel_contributions.

Screen Shot 2020-07-28 at 2 47 45 PM

Ergo this can be done easily on the form layer with the data it already has

Comments

@civibot
Copy link

civibot bot commented Jul 28, 2020

(Standard links)

This toxic function getRowsElementsAndInfo does things for the form layer but it's also heavily used
in the BAO dedupeProcess. Ideally we want the things that are onnly being done to support (one specific) form
to sit on that form. This does that just for one of the 2 arrays it creates of form elements.

The rel_table_elements array is just an array that looks like

[
  ['checkbox',  'move_rel_contributions'],
  ['checkbox, 'move_rel_activities'],
]

The rel_tables array is an array of metadata keyed by eg. move_rel_contributions.

Ergo this can be done easily on the form layer with the data it already has
@eileenmcnaughton
Copy link
Contributor Author

@JKingsnorth @pfigel any chance one of you could review?

Copy link
Contributor

@pfigel pfigel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(CiviCRM Review Template WORD-1.2)

  • General standards
    • (r-explain) PASS
    • (r-user) PASS: No user-visible change
    • (r-doc) PASS
    • (r-run) PASS: Tested that selected entities were still moved. Manually verified that it produces the same markup as before. Note: I did notice that deselecting the move options does not have an effect (i.e. contributions are moved regardless), but that doesn't appear to be caused by this PR. I'll file a separate issue.
  • Developer standards
    • (r-tech) PASS: No impact expected
    • (r-code) PASS
    • (r-maint) PASS
    • (r-test) PASS-ish: I don't think we have explicit test coverage for the form side, but that's in line with much of the rest of the codebase.

@eileenmcnaughton
Copy link
Contributor Author

Thanks @pfigel - appreciate it!

@eileenmcnaughton eileenmcnaughton merged commit 0524386 into civicrm:master Aug 5, 2020
@eileenmcnaughton eileenmcnaughton deleted the merge_form branch August 5, 2020 12:18
@totten
Copy link
Member

totten commented Aug 6, 2020

Ditto thanks @pfigel for good review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants